Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969) #12991

Merged
merged 2 commits into from Jul 21, 2020
Merged

fix(heatmap) Skip rendering for data out of axis content in heatmap (#12969) #12991

merged 2 commits into from Jul 21, 2020

Conversation

quillblue
Copy link
Contributor

@quillblue quillblue commented Jul 19, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

While rendering data for heatmap, judgement on whether data item will be out of current axis scale extent were implemented, and those items out of visible range will be skiped.

Fixed issues

#12969

Details

Before: What was the problem?

As original issue states, yAxis labels will be covered by color blocks (data series) in heatmap when min option was applied to xAxis. Actually these blocks should not be rendered since they are not in current extent of xAxis.

Before

After: How is it fixed in this PR?

A judgement before rendering rect for each data item was added to calculate whether it will be in the extent, and rendering will be skipped if it is not in the extent.
This change also passed tests for scale changes cases such as using dataZoom control, using data zoom in toolbox, etc.
After

Usage

Are there any API changes?

  • The API has been changed.

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

I have to admit that it's not a so perfect solution. A general solution for handling this case (judge whether dataItem is in the scale of current displayed coord) will sure be better but failed to find one, since I found that different series handle such kind of cases in different ways and hard to find a general one also applicable to heatmap. Feel free to comment if have any better idea.

@echarts-bot
Copy link

echarts-bot bot commented Jul 19, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

// Ignore empty data
if (isNaN(data.get(dataDims[2], idx) as number)) {
// Ignore empty data and out of extent data
if (isNaN(data.get(dataDims[2], idx) as number) || data.get(dataDims[0], idx) < xAxisExtent[0] || data.get(dataDims[0], idx) > xAxisExtent[1]) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data.get(dataDims[0], idx) is better to be assigned to a temporal variable.

Also, yAxis should also be checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, have modified accordingly.

@@ -19,91 +18,156 @@
-->

<html>
<head>
Copy link
Contributor

@pissang pissang Jul 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should create a new test case instead of modifying the exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the new case is almost seprate except under same page and share same data. Changes are more likely a false detection (may be due to formatter, actually I didn't change the original case any)

@pissang
Copy link
Contributor

pissang commented Jul 21, 2020

LGTM now

@pissang pissang merged commit 39bd739 into apache:next Jul 21, 2020
@echarts-bot
Copy link

echarts-bot bot commented Jul 21, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@quillblue quillblue deleted the fix-12969 branch August 27, 2020 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants